perf(table): compute partition residuals for local scan tasks - #1971
Conversation
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
635798d to
a8d1268
Compare
Signed-off-by: Minh Vu <vuhoangminh97@gmail.com>
zeroshade
left a comment
There was a problem hiding this comment.
The residual path is conservatively wired through planning, task-specific column projection, and Arrow row filtering. Approving, with one test-matrix follow-up.
Residual computation is the kind of change where being slightly wrong in either direction is silent: too weak returns rows that should have been filtered, too strong drops matching rows. So I focused on the conservatism of the projection rules:
- Identity elision is safe — the partition value fully determines the predicate, so dropping it is correct.
- Lossy transforms stay conservative. Truncate and bucket retain the original predicate rather than eliding it, which is the required behaviour since the transform doesn't uniquely determine the source value.
- Boolean composition — AND/OR/NOT simplification behaves correctly over the mixed cases.
- Nulls and NaN are preserved rather than silently folded, including null partitions, null predicates, NaN partitions and NaN literals.
- Missing partition values and unsupported cases leave the expression unchanged, which is the right default.
- Column projection only narrows to fields the task residual actually needs, while retaining original-filter fields for nil-residual tasks — so a task that didn't get a residual still filters on everything it did before.
- An end-to-end identity residual read covers the whole path rather than just the expression algebra.
Minor — extend the transform matrix (table/partition_residual_test.go:226)
The matrix covers identity, truncate, bucket, and day/time boundaries, but not year, month, or hour with matching, interior, and non-matching partition values. These delegate through the same shared time-transform projection path, so I don't suspect a defect — but a boundary regression in one of them would silently change which rows get scanned, and that's precisely the failure mode residuals can hide. Please add the same boundary and equivalence cases for year/month/hour (including timestamp and timestamp-ns inputs where supported), ideally with an end-to-end read for one of them. Fine as a follow-up.
Benchmarks are committed for both the scanner and residual paths, with body numbers on M1 Pro / Go 1.26.3 / GOMAXPROCS=4 covering identity-only and mixed-filter comparisons.
The one failing check is the known pre-existing SQLite flake #1793 in catalog/sql, which this diff doesn't touch — unrelated, and nothing for you to do. All 4 commits signed off.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Go maintainer, who has read the findings and signed off. If something feels off, please reply on the PR and a maintainer will follow up.
More on how to contribute to Apache Iceberg Go: CONTRIBUTING.md
What
AlwaysTrue,AlwaysFalse, or the remaining row filter for each task.Example
For
ts >= ... AND ts < ...onday(ts)partitions:AlwaysTrue.AlwaysFalse.Benchmark
Apple M1 Pro, Go 1.26.3,
GOMAXPROCS=4. Medians of six 1-second runs on2c72b57.Arrow reads: 32,768 rows from in-memory Parquet, selecting only
payload.AlwaysTrueResidual evaluation for 4,096 files, reusing one evaluator:
Checks
go test ./table/... -count=1go test -race ./table -count=1go test ./... -run=^$ -count=1go vet ./...go run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.12.2 run --timeout=10m🔥